Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide feedback when using deprecated %d specifiers in RUNPATH #4474

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Dec 12, 2022

Issue
Resolves #4432
Resolves #4443

Updated docs
Updated deprecated syntax for RUNPATH in ert-files, tests and examples

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch 2 times, most recently from c6905b9 to cdb1085 Compare December 13, 2022 11:41
@andreas-el andreas-el marked this pull request as ready for review December 13, 2022 11:42
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch 5 times, most recently from 2e43282 to 541d7db Compare December 16, 2022 11:41
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch from 498a5ca to 5471f1c Compare December 16, 2022 14:07
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch from 58be1eb to f31883d Compare December 22, 2022 14:30
@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2023

Codecov Report

Merging #4474 (e2dd788) into main (3edfc6b) will increase coverage by 0.02%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main    #4474      +/-   ##
==========================================
+ Coverage   59.65%   59.67%   +0.02%     
==========================================
  Files         444      444              
  Lines       31326    31313      -13     
  Branches     3076     3076              
==========================================
- Hits        18686    18685       -1     
+ Misses      11866    11854      -12     
  Partials      774      774              
Impacted Files Coverage Δ
...c/ert/gui/tools/load_results/load_results_panel.py 20.27% <0.00%> (+0.04%) ⬆️
src/ert/_c_wrappers/enkf/model_config.py 89.28% <100.00%> (+1.53%) ⬆️
...rc/ert/gui/tools/load_results/load_results_tool.py 60.00% <100.00%> (-10.59%) ⬇️
src/ert/libres_facade.py 88.11% <100.00%> (+0.13%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andreas-el andreas-el added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Jan 2, 2023
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch from 8252c6b to 51bcde8 Compare January 2, 2023 14:25
@andreas-el andreas-el changed the title Add feedback when no %d specifiers found in RUNPATH Provide feedback when using deprecated %d specifiers in RUNPATH Jan 2, 2023
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch 4 times, most recently from 86e5c77 to 2cb7f99 Compare January 3, 2023 07:26
elif not any(x in runpath_format_string for x in ["<ITER>, <IENS>"]):
self.runpath_format_string = runpath_format_string
logger.error(
"RUNPATH keyword shall use syntax " f"`{self.DEFAULT_RUNPATH}`."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should instead instead of shall ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

And added new issue for suggester ui.
#4543

Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had one comment regarding the wording of a warning.

Currently, this shows "suggestions" via logger.warn. However, for some of these messages it would be great if we could show them in the suggester gui. Perhaps we should spawn a new issue for that?

@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch from 2cb7f99 to acb6346 Compare January 3, 2023 08:30
Remove iteration_count property
Remove obsoleted test
Add LibresFacade testing valid runpaths
Added logger feedback on deprecated syntax for runpath
Update documentation for RUNPATH
Replace jobname specifiers with <ITER> and <IENS>
Before load results manually would check whether runpath was valid, however that just checked for the precense of %d.
Since results can be loaded regardless of %d, this button is now always enabled.
@andreas-el andreas-el force-pushed the runpath_specifier_feedback branch from acb6346 to e2dd788 Compare January 3, 2023 08:50
@andreas-el andreas-el merged commit 5286ecf into equinor:main Jan 3, 2023
@andreas-el andreas-el deleted the runpath_specifier_feedback branch January 3, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make runpath file format consistent Add deprecation warning when parsing the RUNPATH keyword
3 participants